Several fixes when setting revision #40
Conversation
massonju
commented
Sep 11, 2024
massonju
commented
- return right revision when git ls-remote
- set revision according to remote url and branch
massonju
force-pushed
the
jmasson/fix-revision
branch
from
4d42509 to
5455832
Compare
When we execute git ls-remote, the pattern is interpreted as a glob [1]. Thus we may retrieve the revision of several remote branchs and we returned the revision of the first branch found. But the first branch may not be the right branch. Example with a remote containing these branchs: a/rev rev If we git ls-remote 'rev', we would have returned the revision of a/rev branch. The issue is now fixed and we return the revision of the requested branch. [1] https://git-scm.com/docs/git-ls-remote/en#Documentation/git-ls-remote.txt-ltpatternsgt82308203 Signed-off-by: Julien Masson <jmasson@baylibre.com>
massonju
force-pushed
the
jmasson/fix-revision
branch
from
5455832 to
0c06c34
Compare
The previous implementation was setting the revision of a project according to only the remote url. Indeed getRevision returned a list of url - revision. But that doesn't work if a remote url is used several times with different path/branch. Example: project A: remote url X, branch Y, path project-a-path project B: remote url X, branch Z, path project-b-path With the previous implementation, both projects will get the same revision since they are using same remote url (same entry in the revisionTable). To fix this issue, we now check the remote url AND the branch. getRevision return the url, revision AND the branch, the branch returned doesn't contain any suffix (tags ...). Signed-off-by: Julien Masson <jmasson@baylibre.com>
massonju
force-pushed
the
jmasson/fix-revision
branch
from
0c06c34 to
ec10c92
Compare
|
I'd like to have another pair of eyes on this review, so we will be awaiting for @david-baylibre 's feedback on this before merging. |
There was a problem hiding this comment.
Nice catch on these bugs @massonju and thanks for the patches
Looks good to me
I just added a non-blocking comment for an open discussion on the temporary project table variable
| try: | ||
| with redirect_stdout(sys.stderr): | ||
| # return tuple (remote/project, revision) | ||
| print('Fetching revision for {}/{}...'.format(remote, project)) | ||
| # return tuple (remote/project, branch, revision) |
There was a problem hiding this comment.
It looks weird to return branch as such which is in the function parameters
I wish we could return a tuple like (remote/project/branch, revision), remote/project/branch being a key kind of... it'd make things easier to update revisions in update_manifest, avoiding the for url, branch, rev in revisionList: in update_manifest
but... a branch can contain '/' (slash) character and project name too, so it seems...
So the following 2 would match:
project "a/1" branch "alpha"
project "a" branch "1/alpha"
So that's the right thing to do: get rid of the revisionTable variable. Maybe we could store as revisionTable[projectRemote][project][branch] or something similar in a future release...?
